Skip to content

Correctly revise methods defined in external method tables #904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Correctly revise methods defined in external method tables #904

wants to merge 20 commits into from

Conversation

serenity4
Copy link
Collaborator

@serenity4 serenity4 commented Apr 18, 2025 •
edited
Loading

Fixes #646.

Requires:

To do before merging:

  • Test this "in the wild" with some large packages (SPIRV.jl and GPUCompiler-dependent packages)
  • Update documentation, docstrings and comments.

timholy
timholy approved these changes Apr 19, 2025
View reviewed changes
Copy link
Owner

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I don't think that #815 offers anything that this doesn't, so this PR could close that one unless you disagree.

@serenity4 serenity4 force-pushed the revise-external-methodtables branch from e69dcb5 to da59589 Compare April 21, 2025 20:22
@serenity4 serenity4 force-pushed the revise-external-methodtables branch from 516a1a0 to 024f45c Compare April 25, 2025 19:47
@serenity4 serenity4 marked this pull request as ready for review June 10, 2025 13:18
@serenity4
Copy link
Collaborator Author

I looked into supporting the mapexpr-form of include, and it doesn't strike me that it would require changes to either CodeTracking or LoweredCodeUtils. I'm not done with that, but it's getting a bit involved and I'm a bit low on time these days.

To avoid leaving this PR rotting for too long I think it might be good to move forward with the release of CodeTracking 2.0 and LoweredCodeUtils 4.0 after JuliaDebug/LoweredCodeUtils.jl#125, so we can merge this PR. Then, if the mapexpr support still eventually requires a new breaking release for either, I would hope the disruption to not be too impactful.

@timholy what do you think?

serenity4
serenity4 commented Jun 10, 2025
View reviewed changes
.github/workflows/ci.yml Outdated
@@ -39,6 +39,7 @@ jobs:
version: ${{ matrix.version }}
show-versioninfo: ${{ matrix.version == 'nightly' }}
- uses: julia-actions/cache@v2
- run: julia --project -e 'using Pkg; pkg"add https://github.com/serenity4/JuliaInterpreter.jl#codetracking-v2 https://github.com/timholy/CodeTracking.jl#master https://github.com/serenity4/LoweredCodeUtils.jl#support-external-methodtables"'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be removed before merge.

.github/workflows/ci.yml Outdated
@@ -80,6 +81,7 @@ jobs:
# We also need to pick up the Git tests, but for that we need to `dev` the package
echo "Git tests"
julia --code-coverage=user -e '
using Pkg; pkg"add https://github.com/serenity4/JuliaInterpreter.jl#codetracking-v2 https://github.com/timholy/CodeTracking.jl#master https://github.com/serenity4/LoweredCodeUtils.jl#support-external-methodtables"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be removed before merge.

@serenity4 serenity4 force-pushed the revise-external-methodtables branch from ab8a38e to d302445 Compare June 10, 2025 13:37
@serenity4 serenity4 mentioned this pull request Jun 10, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timholy timholy timholy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revising methods with external method tables fails
2 participants
@serenity4 @timholy